Skip to content

Modernize C++ tests#5386

Closed
cyyever wants to merge 1 commit intopybind:masterfrom
cyyever:fix
Closed

Modernize C++ tests#5386
cyyever wants to merge 1 commit intopybind:masterfrom
cyyever:fix

Conversation

@cyyever
Copy link
Contributor

@cyyever cyyever commented Sep 29, 2024

Description

Modernize test code.

@cyyever cyyever changed the title Modernize code Modernize C++ tests Sep 29, 2024
@cyyever cyyever marked this pull request as draft September 29, 2024 15:48
@rwgk
Copy link
Collaborator

rwgk commented Sep 29, 2024

I'd rather not make these changes.

E.g.

@@ -92,8 +93,8 @@ TEST_SUBMODULE(class_, m) {
     // test_inheritance
     class Pet {
     public:
-        Pet(const std::string &name, const std::string &species)
-            : m_name(name), m_species(species) {}
+        Pet(std::string name, std::string species)
+            : m_name(std::move(name)), m_species(std::move(species)) {}
         std::string name() const { return m_name; }
         std::string species() const { return m_species; }

That a loss of readability IMO. const in the signature is clear. Scattering std::move() around in the implementation is just clutter in comparison.

Is this a big deal one way or another? — I don't think so. Better leave as is. Better invest time in something that matters more.

@cyyever cyyever closed this Sep 30, 2024
@cyyever cyyever deleted the fix branch September 30, 2024 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments